Remove encryption legacy#21781
Conversation
…back decryptVersioned now throws on non-enc:v2 envelopes instead of falling back to legacy AES-CTR. Update the application-variable and application-registration-variable integration tests to assert the live read path surfaces an error for legacy ciphertext rather than transparently decrypting them.
TODOs/FIXMEs:
|
🔍 Automated Pre-Review✅ No issues detected - This PR is ready for human review. Automated pre-review — human approval still required. |
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
🚀 Preview Environment Ready! Your preview environment is available at: https://baker-entrance-literacy-solomon.trycloudflare.com This environment will automatically shut down after 5 hours. |
Weiko
left a comment
There was a problem hiding this comment.
Code LGTM but I'll let @charlesBochet validate if we can deprecate or not as I didn't really follow that track 👍
charlesBochet
left a comment
There was a problem hiding this comment.
TBH,
- I would just rename decryptVersioned to decryptWithFallbackForUpgradeCommandDeprecated. I would not touch upgrade command (only this renaming in the calls)
- I would call decrypt in the production code
That's should be a few line of changes and not such a big change
| }): PlaintextString { | ||
| return this.secretEncryptionService.decryptVersionedOrThrow(storedSecret, { | ||
| workspaceId, | ||
| }); |
There was a problem hiding this comment.
Bug: Unit tests in two-factor-authentication.service.spec.ts mock decryptVersionedWithLegacyFallback, but the production code now calls decryptVersionedOrThrow, leading to incorrect test coverage.
Severity: LOW
Suggested Fix
Update the unit tests in two-factor-authentication.service.spec.ts to mock decryptVersionedOrThrow instead of decryptVersionedWithLegacyFallback. This will align the test mocks with the actual implementation in the service and ensure the tests are correctly validating the code's behavior.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.ts#L49-L52
Potential issue: The unit tests for the `TwoFactorAuthenticationService` are mocking the
wrong function. The tests set up a mock for `decryptVersionedWithLegacyFallback`, but
the service's implementation was updated to call `decryptVersionedOrThrow` instead. This
discrepancy means the tests are not correctly verifying the behavior of the new code
path and may produce misleading results, as the actual function call to
`decryptVersionedOrThrow` is not being mocked.
Also affects:
packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts:93~99packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts:296~310packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts:433~442
There was a problem hiding this comment.
5 issues found across 25 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/utils/build-env-var.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/utils/build-env-var.ts:19">
P2: Decryption is still prefix-gated, so `buildEnvVar` does not always attempt decrypt and silently passes through non-`enc:v2` values. This defeats the strict runtime read path intended by `decryptVersionedOrThrow`.
(Based on your team's feedback about always decrypting in buildEnvVar without envelope-prefix gating.) [FEEDBACK_USED].</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts:85">
P0: Test mock/expectations renamed to decryptVersionedWithLegacyFallback but production code calls decryptVersionedOrThrow. Tests will fail because the mock object won't have decryptVersionedOrThrow defined.</violation>
</file>
<file name="packages/twenty-server/src/database/commands/secret-encryption-rotation/handlers/sensitive-config-storage-rotation.handler.ts">
<violation number="1" location="packages/twenty-server/src/database/commands/secret-encryption-rotation/handlers/sensitive-config-storage-rotation.handler.ts:121">
P1: Legacy fallback is unreachable for unprefixed ciphertext because the precheck still rejects non-`enc:` values. Cross-upgrade rotation can keep failing/marking errors for legacy rows instead of re-encrypting them.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts:96">
P0: Mock provides `decryptVersionedWithLegacyFallback` but service calls `decryptVersionedOrThrow`. Tests will fail at runtime because `decryptVersionedOrThrow` is `undefined` on the mock.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/utils/__tests__/build-env-var.spec.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/utils/__tests__/build-env-var.spec.ts:15">
P0: Test mock provides `decryptVersionedWithLegacyFallback` but `buildEnvVar` calls `decryptVersionedOrThrow` — tests will throw TypeError at runtime</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| provide: SecretEncryptionService, | ||
| useValue: { | ||
| decryptVersioned: jest.fn((value) => value), | ||
| decryptVersionedWithLegacyFallback: jest.fn((value) => value), |
There was a problem hiding this comment.
P0: Test mock/expectations renamed to decryptVersionedWithLegacyFallback but production code calls decryptVersionedOrThrow. Tests will fail because the mock object won't have decryptVersionedOrThrow defined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts, line 85:
<comment>Test mock/expectations renamed to decryptVersionedWithLegacyFallback but production code calls decryptVersionedOrThrow. Tests will fail because the mock object won't have decryptVersionedOrThrow defined.</comment>
<file context>
@@ -82,7 +82,7 @@ describe('ConfigStorageService', () => {
provide: SecretEncryptionService,
useValue: {
- decryptVersioned: jest.fn((value) => value),
+ decryptVersionedWithLegacyFallback: jest.fn((value) => value),
encryptVersioned: jest.fn((value) => value),
},
</file context>
| provide: SimpleSecretEncryptionUtil, | ||
| useValue: { | ||
| decryptSecret: jest.fn(), | ||
| decryptVersionedWithLegacyFallback: jest.fn(), |
There was a problem hiding this comment.
P0: Mock provides decryptVersionedWithLegacyFallback but service calls decryptVersionedOrThrow. Tests will fail at runtime because decryptVersionedOrThrow is undefined on the mock.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/two-factor-authentication/two-factor-authentication.service.spec.ts, line 96:
<comment>Mock provides `decryptVersionedWithLegacyFallback` but service calls `decryptVersionedOrThrow`. Tests will fail at runtime because `decryptVersionedOrThrow` is `undefined` on the mock.</comment>
<file context>
@@ -93,7 +93,7 @@ describe('TwoFactorAuthenticationService', () => {
useValue: {
encryptVersioned: jest.fn(),
- decryptVersioned: jest.fn(),
+ decryptVersionedWithLegacyFallback: jest.fn(),
},
},
</file context>
| `enc:v2:deadbeef:${value}|${opts?.workspaceId ?? 'instance'}`, | ||
| ), | ||
| decryptVersioned: jest.fn( | ||
| decryptVersionedWithLegacyFallback: jest.fn( |
There was a problem hiding this comment.
P0: Test mock provides decryptVersionedWithLegacyFallback but buildEnvVar calls decryptVersionedOrThrow — tests will throw TypeError at runtime
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/utils/__tests__/build-env-var.spec.ts, line 15:
<comment>Test mock provides `decryptVersionedWithLegacyFallback` but `buildEnvVar` calls `decryptVersionedOrThrow` — tests will throw TypeError at runtime</comment>
<file context>
@@ -12,7 +12,7 @@ describe('buildEnvVar', () => {
`enc:v2:deadbeef:${value}|${opts?.workspaceId ?? 'instance'}`,
),
- decryptVersioned: jest.fn(
+ decryptVersionedWithLegacyFallback: jest.fn(
(value: string, _opts?: { workspaceId?: string }) =>
value.replace(/^enc:v2:[0-9a-f]+:/, '').replace(/\|.*$/, ''),
</file context>
|
|
||
| try { | ||
| const plaintext = this.secretEncryptionService.decryptVersioned(rawValue); | ||
| const plaintext = this.secretEncryptionService.decryptVersionedWithLegacyFallback(rawValue); |
There was a problem hiding this comment.
P1: Legacy fallback is unreachable for unprefixed ciphertext because the precheck still rejects non-enc: values. Cross-upgrade rotation can keep failing/marking errors for legacy rows instead of re-encrypting them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/database/commands/secret-encryption-rotation/handlers/sensitive-config-storage-rotation.handler.ts, line 121:
<comment>Legacy fallback is unreachable for unprefixed ciphertext because the precheck still rejects non-`enc:` values. Cross-upgrade rotation can keep failing/marking errors for legacy rows instead of re-encrypting them.</comment>
<file context>
@@ -118,7 +118,7 @@ export class SensitiveConfigStorageRotationHandler extends SecretEncryptionRotat
try {
- const plaintext = this.secretEncryptionService.decryptVersioned(rawValue);
+ const plaintext = this.secretEncryptionService.decryptVersionedWithLegacyFallback(rawValue);
const reEncrypted =
this.secretEncryptionService.encryptVersioned(plaintext);
</file context>
| acc[flatApplicationVariable.key] = | ||
| isNonEmptyString(value) && isEncryptedString(value) | ||
| ? secretEncryptionService.decryptVersioned(value, { | ||
| ? secretEncryptionService.decryptVersionedOrThrow(value, { |
There was a problem hiding this comment.
P2: Decryption is still prefix-gated, so buildEnvVar does not always attempt decrypt and silently passes through non-enc:v2 values. This defeats the strict runtime read path intended by decryptVersionedOrThrow.
(Based on your team's feedback about always decrypting in buildEnvVar without envelope-prefix gating.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/logic-function/logic-function-executor/utils/build-env-var.ts, line 19:
<comment>Decryption is still prefix-gated, so `buildEnvVar` does not always attempt decrypt and silently passes through non-`enc:v2` values. This defeats the strict runtime read path intended by `decryptVersionedOrThrow`.
(Based on your team's feedback about always decrypting in buildEnvVar without envelope-prefix gating.) .</comment>
<file context>
@@ -16,7 +16,7 @@ export const buildEnvVar = (
acc[flatApplicationVariable.key] =
isNonEmptyString(value) && isEncryptedString(value)
- ? secretEncryptionService.decryptVersioned(value, {
+ ? secretEncryptionService.decryptVersionedOrThrow(value, {
workspaceId: flatApplicationVariable.workspaceId,
})
</file context>
Introduction
Still preserving the cross-upgrade flow
close twentyhq/core-team-issues#2465